Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/ns1: No splitting answer on spf records. #13260

Merged
merged 2 commits into from
Apr 8, 2017

Conversation

pashap
Copy link
Contributor

@pashap pashap commented Mar 31, 2017

References #12758

Previously, provider was incorrectly splitting spf answers by spaces.

Tested via host -t SPF terraform.example dns1.p03.nsone.net and received the expected output

@radeksimko
Copy link
Member

radeksimko commented Apr 1, 2017

Hi @pashap
thanks for submitting the PR.

Would you mind attaching an acceptance test too? The test would normally go into builtin/providers/ns1/resource_record_test.go - It should be fairly easy to take one of the existing tests and modify according to our needs here for this bugfix, but let me know if you need further help in that area.

Checking the API response struct (via testAccCheckRecordAnswerRdata) and/or state (via resource.TestCheckResourceAttr) is sufficient - i.e. we don't need to wait for the DNS record to propagate nor use the DNS protocol in this context generally.

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Apr 1, 2017
@pashap
Copy link
Contributor Author

pashap commented Apr 7, 2017

Hi @radeksimko, thanks for pointing that out. Ill avoid PRs in the future without acceptance tests, they are very easy to write in terraform :)

I just pushed a commit with the SPF record acc test, please let me know if it looks good whenever you get a chance

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Apr 7, 2017
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for adding the test.

@radeksimko radeksimko merged commit 8e87b10 into hashicorp:master Apr 8, 2017
@pashap pashap deleted the b-ns1-provider-spf-record-fix branch April 10, 2017 16:14
@ghost
Copy link

ghost commented Apr 14, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants